refactor(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests #3871

HappyZombies · 2025-10-17T18:41:43Z

This PR does the following:

Test: Encodes MYSQL username/password when constructing the config URI in common.test.cjs to prevent malformed URIs with reserved characters. This is to prevent the issue where a username and/or password could contain special characters and it not get parsed properly (e.g., username is "engineer:frontend", "engineer" would be passed as the username, not "engineer:frontend).
Test: Adds a targeted .catch() in some test files, this ensures teardown (e.g., connection.end()) runs reliably.
Docs: Update Contributing guide to clearly state the database name MUST be exactly "test" when running the suite.

Please NOTE:

The added .catch() in these two files do not cover all other test suites with similar patterns (e.g., if they fail, the tests will hang). It aims to address the most common pitfall (at least that I believe), which are failures caused by using the wrong DB name when running test suites, due to some hard coded "test" assertion tests.

I believe that a comprehensive fix would require a broader refactor across all tests to unify error handling and proper resource teardown (e.g., actually calling connection.end()).

So point is, this is a temporary for the test files with the .catch() on it, so I am not opposed if they want to be removed/not added for a better refactor down the line. :)

Address: #3859 and #3854

… database name must be called "test"

…chema/db name, to catch and not hang if the assertion fails.

codecov · 2025-10-19T09:37:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.78%. Comparing base (38f3748) to head (c53d0ec).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3871   +/-   ##
=======================================
  Coverage   89.78%   89.78%           
=======================================
  Files          86       86           
  Lines       13607    13607           
  Branches     1607     1607           
=======================================
  Hits        12217    12217           
  Misses       1390     1390

Flag	Coverage Δ
compression-0	`88.89% <ø> (ø)`
compression-1	`89.76% <ø> (ø)`
static-parser-0	`87.35% <ø> (ø)`
static-parser-1	`88.12% <ø> (ø)`
tls-0	`89.18% <ø> (ø)`
tls-1	`89.55% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

wellwelwel

Thanks, @HappyZombies!

Using catch for errors raises some concerns for me, especially because it can ignore errors when they actually need to be flagged.

We can use the describe + it set instead, for example:

Before:

await connect();

assert(false); // throws

await disconnect(); // this will not be performed

New:

await describe('...', async () => {
  await connect();

  it('...', () => {
    assert(false); // throws within the `it` scope
  });

   await disconnect(); // this will be performed
});

In practice, you can change from this (a behavior particular to Poku):

describe('...', common.describeOptions);

To this (widely adopted standard in testing):

await describe('...', async () => {
  // ...
});

Tip

In ES Module tests, there is no need for (async () => {})() 🙋🏻‍♂️
Related:
- https://poku.io/docs/documentation/helpers/describe
- https://poku.io/docs/documentation/helpers/it

Contributing.md

HappyZombies · 2025-10-20T18:34:55Z

await describe('...', async () => {
  // ...
});

@wellwelwel this pattern still doesn't seem to work under this particular context, looking at test-column-inspect-test, I refactored it to be the following.

import { test, assert, describe, beforeEach, it } from 'poku';
import util from 'node:util';
import { createRequire } from 'node:module';

const require = createRequire(import.meta.url);
const common = require('../../../common.test.cjs');

await describe('Custom inspect for column definition', async () => {
  const connection = common.createConnection().promise();

  it('maps fields to schema-like description when depth > 1', async () => {
    const schema = `
        id INT NOT NULL AUTO_INCREMENT,
        weight INT(2) UNSIGNED ZEROFILL,
        usignedInt INT UNSIGNED NOT NULL,
        signedInt INT NOT NULL,
        unsignedShort SMALLINT UNSIGNED NOT NULL,
        signedShort SMALLINT NOT NULL,
        tinyIntUnsigned TINYINT UNSIGNED NOT NULL,
        tinyIntSigned TINYINT NOT NULL,
        mediumIntUnsigned MEDIUMINT UNSIGNED NOT NULL,
        mediumIntSigned MEDIUMINT NOT NULL,
        bigIntSigned BIGINT NOT NULL,
        bigIntUnsigned BIGINT UNSIGNED NOT NULL,
        longText_ LONGTEXT NOT NULL,
        mediumText_ MEDIUMTEXT NOT NULL,
        text_ TEXT NOT NULL,
        tinyText_ TINYTEXT NOT NULL,
        varString_1000 VARCHAR(1000) NOT NULL,
        decimalDefault DECIMAL,
        decimal13_10 DECIMAL(13,10),
        floatDefault FLOAT,
        float11_7 FLOAT(11,7),
        dummyLastFieldToAllowForTrailingComma INT,
      `;

    await connection.query(
      `CREATE TEMPORARY TABLE test_fields (${schema} PRIMARY KEY (id))`
    );

    const [, columns] = await connection.query('select * from test_fields');
    const inspectResults = util.inspect(columns);
    const schemaArray = schema
      .split('\n')
      .map((line) => line.trim())
      .filter((line) => line.length > 0)
      .map((line) => {
        const words = line.split(' ');
        const name = `\`${words[0]}\``;
        return [name, ...words.slice(1)].join(' ');
      });

    const normalizedInspectResults = inspectResults
      .split('\n')
      .slice(1, -2) // remove "[" and "]" lines and also last dummy field
      .map((line) => line.trim())
      // remove primary key - it's not in the schema explicitly but worth having in inspect
      .map((line) => line.split('PRIMARY KEY ').join(''));

    for (let l = 0; l < normalizedInspectResults.length; l++) {
      const inspectLine = normalizedInspectResults[l];
      const schemaLine = schemaArray[l];

      assert.equal(
        inspectLine,
        schemaLine,
        'Loop: Should map fields to a schema-like description when depth is > 1'
      );
    }
  });

  it('shows detailed description when depth < 1', async () => {
    await connection.query(`
        CREATE TEMPORARY TABLE test_fields2 (
            id INT,
            decimal13_10 DECIMAL(13,10) UNSIGNED NOT NULL,
            PRIMARY KEY (id)
        )
      `);

    const [, columns] = await connection.query('select * from test_fields2');
    const inspectResults = util.inspect(columns[1]);

    assert.deepEqual(
      inspectResults,
      util.inspect({
        catalog: 'def',
        schema: 'test',
        name: 'decimal13_10',
        orgName: 'decimal13_10',
        table: 'test_fields2',
        orgTable: 'test_fields2',
        characterSet: 63,
        encoding: 'binary',
        columnLength: 14,
        type: 246,
        flags: ['NOT NULL'],
        decimals: 10,
        typeName: 'NEWDECIMAL',
      }),
      'should show detailed description when depth is < 1'
    );
  });

  await connection.end();
});

Notice that the callback for the it method is an async query, due to this, the await connection.end() gets called first and causes a race case, that being the connection terminates first before the async callback is ran, this results in the following error:

unhandledRejection Error: Can't add new command when connection is in closed state

Now adding await in front of the it method fixes this problem, but it re-introduces the .connection.end() method never being called.

await connect();

assert(false); // throws

await disconnect(); // this will not be performed

It seems like when the throw occurs inside the it scope (when await is added in the front) it never logs the error since the connection is still alive.

Maybe we can add some way in the commons file to register the connections and disconnect if an unhandledRejection occurs? reason why I added the catch since that is what is happening, an uncaught or unhandledRejection is occurring and not being caught since the connection to the DB is alive/not releasing the event loop.

Does Poku have something like this so I can try? I don't think it does -- idea here would be to simply connect and disconnect after ever run, but then again idk iit would really solve the problem.

 await beforeAll(async () => {
    connection = common.createConnection().promise();
    await connection.connect?.(); // fully wait to be connected first
  });

  await afterAll(async () => {
    await connection.end();
  });

I wonder how other test frameworks have solved this issue if at all? Or maybe your test framework has a sense of a timeout so if any error throws it can move on? Idk, point is, I can't get this to work how you want cuz it doesn't work! haha, I really think I need try/catches here!

wellwelwel · 2025-10-20T19:13:52Z

unhandledRejection Error: Can't add new command when connection is in closed state

I believe the condition you mentioned is because you are not waiting for the result of the it methods (async/await).

It seems like when the throw occurs inside the it scope (when await is added in the front) it never logs the error since the connection is still alive.

I took your last example and ensured the await in the it methods and also used the beforeEach and afterEach hooks to clean up and close the connection after each test (it) cycle. @HappyZombies, can you try this way?

import { assert, describe, afterEach, beforeEach, it } from 'poku';
import util from 'node:util';
import { createRequire } from 'node:module';

const require = createRequire(import.meta.url);
const common = require('../../../common.test.cjs');

await describe('Custom inspect for column definition', async () => {
  let connection;

  beforeEach(async () => {
    connection = common.createConnection().promise();
    await connection.query(`DROP TABLE IF EXISTS test_fields`);
  });

  afterEach(async () => {
    await connection.end();
  });

  await it('maps fields to schema-like description when depth > 1', async () => {
    const schema = `
        id INT NOT NULL AUTO_INCREMENT,
        weight INT(2) UNSIGNED ZEROFILL,
        usignedInt INT UNSIGNED NOT NULL,
        signedInt INT NOT NULL,
        unsignedShort SMALLINT UNSIGNED NOT NULL,
        signedShort SMALLINT NOT NULL,
        tinyIntUnsigned TINYINT UNSIGNED NOT NULL,
        tinyIntSigned TINYINT NOT NULL,
        mediumIntUnsigned MEDIUMINT UNSIGNED NOT NULL,
        mediumIntSigned MEDIUMINT NOT NULL,
        bigIntSigned BIGINT NOT NULL,
        bigIntUnsigned BIGINT UNSIGNED NOT NULL,
        longText_ LONGTEXT NOT NULL,
        mediumText_ MEDIUMTEXT NOT NULL,
        text_ TEXT NOT NULL,
        tinyText_ TINYTEXT NOT NULL,
        varString_1000 VARCHAR(1000) NOT NULL,
        decimalDefault DECIMAL,
        decimal13_10 DECIMAL(13,10),
        floatDefault FLOAT,
        float11_7 FLOAT(11,7),
        dummyLastFieldToAllowForTrailingComma INT,
      `;

    await connection.query(
      `CREATE TEMPORARY TABLE test_fields (${schema} PRIMARY KEY (id))`
    );

    const [, columns] = await connection.query('select * from test_fields');
    const inspectResults = util.inspect(columns);
    const schemaArray = schema
      .split('\n')
      .map((line) => line.trim())
      .filter((line) => line.length > 0)
      .map((line) => {
        const words = line.split(' ');
        const name = `\`${words[0]}\``;
        return [name, ...words.slice(1)].join(' ');
      });

    const normalizedInspectResults = inspectResults
      .split('\n')
      .slice(1, -2) // remove "[" and "]" lines and also last dummy field
      .map((line) => line.trim())
      // remove primary key - it's not in the schema explicitly but worth having in inspect
      .map((line) => line.split('PRIMARY KEY ').join(''));

    for (let l = 0; l < normalizedInspectResults.length; l++) {
      const inspectLine = normalizedInspectResults[l];
      const schemaLine = schemaArray[l];

      assert.equal(
        inspectLine,
        schemaLine,
        'Loop: Should map fields to a schema-like description when depth is > 1'
      );
    }
  });

  await it('shows detailed description when depth < 1', async () => {
    await connection.query(`
        CREATE TEMPORARY TABLE test_fields2 (
            id INT,
            decimal13_10 DECIMAL(13,10) UNSIGNED NOT NULL,
            PRIMARY KEY (id)
        )
      `);

    const [, columns] = await connection.query('select * from test_fields2');
    const inspectResults = util.inspect(columns[1]);

    assert.deepEqual(
      inspectResults,
      util.inspect({
        catalog: 'def',
        schema: 'test',
        name: 'decimal13_10',
        orgName: 'decimal13_10',
        table: 'test_fields2',
        orgTable: 'test_fields2',
        characterSet: 63,
        encoding: 'binary',
        columnLength: 14,
        type: 246,
        flags: ['NOT NULL'],
        decimals: 10,
        typeName: 'NEWDECIMAL',
      }),
      'should show detailed description when depth is < 1'
    );
  });
});

Success Example	Failure Example

HappyZombies · 2025-10-20T19:18:35Z

@wellwelwel mhm yup that seems to work, the key thing here was using adding the after each hook, not just calling .end() at the end of the tests.

  afterEach(async () => {
    await connection.end();
  });

And of course, updating the beforeEach() to connect again. Ok this will work then, guess I was really hellbent on keeping the test as it was.

I think a lot more unit tests could benefit from this, however for this PR I will start with these two that I added and look at fixing others in another PR.

wellwelwel · 2025-10-20T20:12:13Z

Thanks again, @HappyZombies! ✨

HappyZombies added 3 commits October 15, 2025 11:52

fix(docs): improve the contribution guidelines to state that the test…

3e838af

… database name must be called "test"

fix(test): fix tests, particularly those with hardcoded test as the s…

2206c68

…chema/db name, to catch and not hang if the assertion fails.

fix(test): Fix URI encoding for user/password in common.test.config

a6d7aee

wellwelwel requested changes Oct 20, 2025

View reviewed changes

Contributing.md Outdated Show resolved Hide resolved

wellwelwel changed the title ~~fix(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests~~ refactor(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests Oct 20, 2025

HappyZombies added 2 commits October 20, 2025 11:42

fix(docs): address code review comments suggestion

91c8f57

fix(docs): address code review comments suggestion, remove extra space

fc6a308

HappyZombies added 2 commits October 20, 2025 15:33

fix(tests): refactor unit tests as per code review

a4d74ac

fix(tests): remove unnecessary async callback

abfa718

HappyZombies requested a review from wellwelwel October 20, 2025 19:36

Merge branch 'master' into refactor.unit-test-resource-leak-adjustment

c53d0ec

wellwelwel added refactor documentation labels Oct 20, 2025

wellwelwel approved these changes Oct 20, 2025

View reviewed changes

wellwelwel merged commit 3714220 into sidorares:master Oct 20, 2025
100 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests #3871

refactor(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests #3871

HappyZombies commented Oct 17, 2025 •

edited

Loading

Uh oh!

codecov bot commented Oct 19, 2025 •

edited

Loading

Uh oh!

wellwelwel left a comment •

edited

Loading

Uh oh!

Uh oh!

HappyZombies commented Oct 20, 2025

Uh oh!

wellwelwel commented Oct 20, 2025 •

edited

Loading

Uh oh!

HappyZombies commented Oct 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

wellwelwel commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

refactor(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests #3871

refactor(docs,test): small clarification update on contribution guidelines and fix URI-encoding + hardcoded-db tests #3871

Conversation

HappyZombies commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wellwelwel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HappyZombies commented Oct 20, 2025

Uh oh!

wellwelwel commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HappyZombies commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wellwelwel commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HappyZombies commented Oct 17, 2025 •

edited

Loading

codecov bot commented Oct 19, 2025 •

edited

Loading

wellwelwel left a comment •

edited

Loading

wellwelwel commented Oct 20, 2025 •

edited

Loading

HappyZombies commented Oct 20, 2025 •

edited

Loading